Skip to content

Conversation

janwinkler1
Copy link
Contributor

Fixes target naming to follow symbolic macro conventions introduced in Bazel 8.0.

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution!

  • Is there any way we can add a test for this?
  • Could you please add a CHANGELOG.md note to let people know that this fixes an issue?

@janwinkler1
Copy link
Contributor Author

@aignas thank you for your review!

sure, i am looking into it ATM.

in the dev-guide i read that integration tests are discuraged.

in c91459a i quickly tried to add a simpler build test, however that failed in CI, presumably because bazel 7 doesnt support symbolic macros.

so do you think in this case it would be okay to add a new integration test and ignore (if possible) bazel 7 tests there?

@aignas aignas mentioned this pull request Oct 11, 2025
11 tasks
@rickeylev
Copy link
Collaborator

re: testing this: Is adding target_compatible_with sufficient? Or does bazel 7 failure before it that is processed? If TCW isn't checked early enough, then how about wrapping it in a legacy macro, then checking the bazel version there, and skiping early? There's utility target in rules_testing for generating a no-op skip target for such cases; see skip_test in tests/py_runtime_pair/py_runtime_pair_tests.bzl for an example

re: losing the underscore prefix: We'll just have to live with it. We could add an infix or suffix to help denote its internal. e.g. foo_gen__ foo__gen foo_internal_gen foo_private_gen etc

@aignas aignas changed the title Make py_console_script_binary compatible with symbolic macros fix(rules): make py_console_script_binary compatible with symbolic macros Oct 11, 2025
@aignas aignas enabled auto-merge October 11, 2025 04:48
@aignas aignas added this pull request to the merge queue Oct 11, 2025
Merged via the queue into bazel-contrib:main with commit a2f14eb Oct 11, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants